fix(server): refuse to start a second server against a live data directory - #8442
fix(server): refuse to start a second server against a live data directory#8442NoahLinckeScout wants to merge 5 commits into
Conversation
…ctory
Two servers pointed at one `--base-dir` both open `state.sqlite` and both write
`settings.json`, and they overwrite each other. Observed: a desktop app
auto-updated to a newer server while the old one was still running, the new
process found its port taken, silently bound a random one, and ran blind against
shared state. The visible symptom was a settings toggle that would not stick --
hours away from the cause, and nothing about it is detectable afterwards.
So refuse at startup. The lock is claimed before anything binds a port or opens
the database, and is provided into `HttpServerLive` rather than merged beside it
so the ordering is structural: the lock is a dependency of the thing it protects.
An advisory `flock` would be the better primitive, since the kernel drops it when
the holder dies. Node has no binding for it and a native dependency for one lock
is the worse trade, so this is an atomically created file holding the owner's
identity, with liveness checked by signal 0. The tradeoff is stated in the module:
a killed server whose pid is later reused blocks startup until the file is
removed, which is the safe direction, and the message names the file.
A lock whose owner is gone, or which a crash tore in half mid-write, is reclaimed
rather than treated as permanent. Reclaiming re-races the exclusive create, so two
servers starting together still produce one winner. Release only removes a lock
this process still owns, so a successor is never evicted.
The bound port is stamped onto the lock afterwards purely so a later server's
refusal names an address the user can open rather than just a pid.
Verified end to end against two real servers: the second refuses with the message
below, exits 1, and never binds; shutdown releases; restart is unblocked.
Another T3 Code server is already using this data directory.
data directory: /tmp/t3-smoke-basedir/userdata
held by: pid 285163, listening on port 39977
since: 2026-08-27T16:59:03.329Z
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a006bdbdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Reviewed the new apps/server/src/serverSingleton.ts and its wiring in apps/server/src/server.ts against the Effect service conventions.
Imports (namespace subpath imports), Effect.fn usage, FileSystem/Path acquisition from the environment, scoped acquisition via Effect.acquireRelease inside Layer.effectDiscard, and the new tests all look consistent with the conventions. Two findings in the error-modelling area are commented inline: the underlying PlatformError from the exclusive lock write is discarded instead of being classified/preserved as cause, and ServerLockUnavailableError carries the message as a free-form single-value reason string.
Posted via Macroscope — Effect Service Conventions
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This changes production server startup by adding a persistent per-data-directory lock, heartbeat, stale-lock reclamation, legacy-state handling, and atomic metadata updates. Because the concurrency-sensitive logic can prevent the entire server from binding and gates downstream work, it warrants focused human review despite the narrow intent and extensive tests. You can add or adjust custom eligibility rules. Learn more. |
…r races Review on pingdotgg#8442 found the guard itself re-introduced the corruption it exists to prevent, plus one rollout gap. All were right. - A pre-lock server writes no server.lock, only server-runtime.json with its live pid, so the file was blind to the running 0.0.34 the upgrade swaps out. Read that as a held lock and refuse the same way before anything binds, or the auto-update incident still happens once on upgrade day. - Between one starter reading a stale lock and recreating it, the unconditional unlink removed the successor's fresh claim: two starters after a crash each unlinked the other's lock and both proceeded. Reclaim now refreshes the dead file's mtime across several observation rounds and only removes what stays untouched, and the live holder's own heartbeat refreshes inside one round, so a live claim can never be reclaimed from under it. - recordServerLockPort rewrote the lock in place, truncating first; a reader in that window decoded an empty holder and reclaimed a live lock. The update now goes through write-temp-then-rename, and release never removes a lock it cannot decode. - Lock-create errors stopped being coerced into "taken": a permission or disk failure used to surface as "another server is running". Only AlreadyExists reads as contention now, and the exhaustion error keeps the observation count instead of fixed prose. Verified: apps/server suite 246 files passed / 2 skipped, 2829 tests passed / 10 skipped (parent: 245 files, 2815 passed). Typecheck exit 0. The new tests also pin the pre-fix behaviour as failing, not just the new behaviour as passing.
There was a problem hiding this comment.
One convention issue found: a single-tag recovery uses Effect.catchTag instead of Effect.catchTags. The earlier findings on Effect.orElseSucceed(() => false) swallowing non-AlreadyExists platform errors and on the prose-only reason field of ServerLockUnavailableError are addressed in this revision.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d9c354d. Configure here.
Macroscope on the previous commit: reclaiming a dead lock returned ServerLockUnavailableError even with no competing starter, so the first restart after a crash failed instead of claiming the freed directory; and a shutdown racing readHolder to remove the lock sent fs.utimes a NotFound that failed the starter outright. A confirmed-dead reclaim now retries the exclusive create on the next pass, so a clean restart after a crash claims its directory in one call. The retry is still bounded — MAX_RECLAIM_CYCLES — so a lock another starter keeps recreating, or a permissions wall keeps failing to remove, surfaces as ServerLockUnavailableError rather than spinning. Both refresh paths tolerate NotFound between the read and the utimes, which closes the shutdown race; the two paths shared a tail and now use one branch. Verified: serverSingleton suite 14/14, typecheck exit 0.
…rst error Cursor Bugbot on the previous commit: Effect.catch sat outside Effect.repeat, and Effect.repeat terminates a failing effect, so the first transient read or utimes error stopped the heartbeat for the rest of the process. Once the heartbeat stops, the lock it protects can be reclaimed out from under a live holder. Recovery now lives inside the round: each tick is caught individually, and the repeat wraps the recovered tick. Verified: serverSingleton suite 14/14, typecheck exit 0.

What Changed
A new
apps/server/src/serverSingleton.tsclaims the data directory at server startup. A second server started against a directory another live server already holds now exits 1 with an explanatory message instead of starting anyway.The lock is
Layer.provided intoHttpServerLiverather than merged beside it, so the ordering is structural rather than incidental: the lock is a dependency of the thing it protects, and no server reaches a listening socket while another holds the directory.Three files, no refactors, no behaviour change for the single-server case. Only
t3 serveandt3 startbuild the server layer, so no other subcommand is affected.Why
Two T3 Code servers pointed at the same
--base-dirboth openstate.sqliteand both writesettings.json, and they overwrite each other. Nothing refuses the second start, and nothing reports it afterwards.The port check does not save you. When
:3775is already taken, the second server binds a different port and starts normally — so it looks perfectly healthy while running blind against shared state.Repro:
Before this change both start. Both hold
/tmp/t3-repro/userdata/state.sqliteopen and both write/tmp/t3-repro/userdata/settings.json; whichever writes last wins, and the other UI silently reverts.How I hit it in the wild: a desktop app auto-updated to a newer server while the old one was still running. The server does not self-upgrade, so
npx t3@<newer>started a second server against the same data directory. The visible symptom was a settings toggle that would not stick — about an hour away from the actual cause, and nothing about it is detectable after the fact.After this change, the second server exits 1 without binding a port:
Why a pid file and not
flockAn advisory
flockis the better primitive — the kernel drops it when the holder dies, so a crash leaves nothing stale. Node has no binding for it, and pulling in a native dependency for one lock seemed the worse trade. So this is a file created atomically withwxholding the owner's identity, with liveness checked via signal 0 (treatingEPERMas alive, since a server running as another user must not be trampled).The tradeoff is stated in the module rather than hidden: a server that is killed and whose pid is later reused by an unrelated process will block startup until the lock file is removed. That is the safe direction to fail, and the message names the file so recovery is one
rm. If you would rather take the native dependency and use a realflock, say so and I will redo it that way.Staleness is handled rather than fatal: a lock whose owner is gone, or which a crash tore in half mid-write, is reclaimed. Reclaiming re-races the exclusive create, so two servers starting simultaneously still produce exactly one winner. Release only removes a lock this process still owns, so a successor is never evicted by its predecessor's shutdown.
The bound port is stamped onto the lock after binding, purely so a later refusal can name an address the user can open rather than a bare pid.
One thing worth flagging for review: the lock is structurally guaranteed to precede the HTTP server binding, which is what the refusal path depends on. I did not attempt to order it against every sibling layer, so I would not claim it strictly precedes the first SQLite open in all compositions.
UI Changes
None — server startup behaviour only.
Tests
apps/server/src/serverSingleton.test.ts— 9 tests, following the existingit.layer(NodeServices.layer)convention used elsewhere in this directory: claim/release on scope exit, refusal while held, stale-pid reclaim, half-written-file reclaim, successor not evicted on release, port recorded and surfaced in the refusal, separate directories independent, and pid liveness.I checked the tests actually bite rather than just passing: changing the exclusive create from
{ flag: "wx" }to{ flag: "w" }fails "refuses a second server while the first holds the directory" and "records the bound port so the next server can name it".Verified locally:
vp test run src/serverSingleton.test.ts→ 9 passedapps/serversuite → 246 files passed, 2824 passed / 10 skipped (baseline on this commit's parent: 245 files, 2815 passed)vp run typecheck→ exit 0vp fmt --check→ clean;vp lintreports nothing new for the changed filesAlso verified end to end with two real server processes against one
--base-dir: the second refuses, exits 1, and never binds; SIGKILLing the holder leaves a stale lock that the next start reclaims; ordinary shutdown releases the lock and a restart is unblocked.Checklist
Note
Medium Risk
Changes server startup ordering and adds filesystem locking with reclaim races, but only blocks the previously unsafe multi-server case and is covered by extensive tests.
Overview
Prevents two T3 Code servers from sharing one
--base-dir(which corruptsstate.sqliteandsettings.json) by claiming<stateDir>/server.lockbefore the HTTP server binds.Startup now acquires the lock via
ServerSingletonLivelayered as a dependency ofHttpServerLive, so a second process fails withServerAlreadyRunningErrorinstead of binding another port and running against shared state. The refusal names the holder’s pid and, after bind, the listening port (recordServerLockPortuses atomic rewrite). Pre-lock servers are detected via live pid inserver-runtime.jsonso desktop auto-update upgrades still refuse a running old server.Stale or crash-torn locks are reclaimed with bounded mtime observation and a holder heartbeat so live locks are not deleted under races; release only removes locks owned by the current pid.
Reviewed by Cursor Bugbot for commit a678dfc. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add server singleton lock to prevent duplicate instances per data directory
ServerSingleton.acquireServerSingletonwhich claims an exclusive lock file in the configured state directory before the HTTP server binds any port, refusing startup when another live process already holds itServerAlreadyRunningErrorreferencing the legacy pathServerAlreadyRunningErrororServerLockUnavailableErrorinstead of starting; all in-tree startup paths inserver.tsnow depend onServerSingletonLiveMacroscope summarized a678dfc.